Skip to content

fix: normalize OAuth redirect URI URL subtypes#2744

Open
he-yufeng wants to merge 1 commit into
modelcontextprotocol:mainfrom
he-yufeng:fix/oauth-redirect-uri-url-subtypes-v2
Open

fix: normalize OAuth redirect URI URL subtypes#2744
he-yufeng wants to merge 1 commit into
modelcontextprotocol:mainfrom
he-yufeng:fix/oauth-redirect-uri-url-subtypes-v2

Conversation

@he-yufeng
Copy link
Copy Markdown

Summary

  • normalize redirect_uris values at the OAuth client metadata boundary
  • keep raw string, AnyUrl, and URL subtype inputs serializing the same way
  • add a regression test for AnyHttpUrl registration followed by AnyUrl redirect validation

To verify

  • .\.venv\Scripts\python.exe -m pytest tests\shared\test_auth.py -q
  • .\.venv\Scripts\python.exe -m ruff check src\mcp\shared\auth.py tests\shared\test_auth.py
  • .\.venv\Scripts\python.exe -m ruff format --check src\mcp\shared\auth.py tests\shared\test_auth.py
  • git diff --check

Refs #2687

@he-yufeng he-yufeng force-pushed the fix/oauth-redirect-uri-url-subtypes-v2 branch from 062b5b4 to ffea4ae Compare May 31, 2026 23:59
Copy link
Copy Markdown

@StantonMatt StantonMatt left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this still misses one input shape Pydantic accepts for this field. redirect_uris is typed as list[AnyUrl], but Pydantic also accepts tuples/sets and coerces them to a list. Because the before validator only normalizes list, a tuple of URL subtypes still stores the element as AnyHttpUrl, so the existing comparison against an incoming AnyUrl can fail:

info = OAuthClientInformationFull(
    client_id="abc123",
    redirect_uris=(AnyHttpUrl("https://example.com/callback"),),
)
info.validate_redirect_uri(AnyUrl("https://example.com/callback"))
# InvalidRedirectUriError: Redirect URI 'https://example.com/callback' not registered for client

I verified the list cases are fixed on ffea4ae; raw-string lists, AnyHttpUrl lists, mixed lists, and single-URI default selection all behave as expected. Local checks also pass:

  • uv run --frozen pytest tests/shared/test_auth.py -q
  • uv run --frozen ruff check src/mcp/shared/auth.py tests/shared/test_auth.py
  • uv run --frozen ruff format --check src/mcp/shared/auth.py tests/shared/test_auth.py
  • uv run --frozen pyright src/mcp/shared/auth.py tests/shared/test_auth.py

Could the normalizer cover the other collection inputs Pydantic accepts here too?

@he-yufeng he-yufeng force-pushed the fix/oauth-redirect-uri-url-subtypes-v2 branch from ffea4ae to 99b80b5 Compare June 4, 2026 20:42
@he-yufeng
Copy link
Copy Markdown
Author

Covered the additional collection inputs in 99b80b5.

The before-validator now normalizes list, tuple, set, and frozenset redirect URI inputs before Pydantic coerces the field, so URL subtype elements compare correctly after validation.

Validation:

uv run --frozen pytest tests/shared/test_auth.py -q
# 16 passed

uv run --frozen ruff check src/mcp/shared/auth.py tests/shared/test_auth.py
# All checks passed

uv run --frozen ruff format --check src/mcp/shared/auth.py tests/shared/test_auth.py
# 2 files already formatted

uv run --frozen pyright src/mcp/shared/auth.py tests/shared/test_auth.py
# 0 errors

python -m py_compile src\mcp\shared\auth.py tests\shared\test_auth.py

git diff --check

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants